Skip to content

improve: remove misleading log message #2891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Aug 8, 2025

This warn message is logged even if missing config is expected

Signed-off-by: Attila Mészáros [email protected]

This warn message is logged even if missing config is expected

Signed-off-by: Attila Mészáros <[email protected]>
@metacosm
Copy link
Collaborator

metacosm commented Aug 8, 2025

I don't think this is the appropriate fix. The appropriate fix would be to override logMissingReconcilerWarning in BaseConfigurationServiceto only output the warning if createIfNeeded returns true. We could simplify the code even more and maybe remove createIfNeeded altogether but this would be an API breaking change (in case someone depends on BaseConfigurationService for their own ConfigurationService implementation).

@csviri
Copy link
Collaborator Author

csviri commented Aug 8, 2025

The problem now is that this warning is logged always, even if everything is ok from JOSDK perspective. I think it is still better to remove it and not confuse users, and finetune it later.

@metacosm
Copy link
Collaborator

metacosm commented Aug 8, 2025

See #2892, the warning should not be output anymore with that implementation

@csviri
Copy link
Collaborator Author

csviri commented Aug 11, 2025

What I fail to see how this message helps the user. If any we should have such log message maybe on debug level that helps development / troubleshooting, but not on warning level. But in addition to that, mainly we should have a test for the API, so configurations are correctly initialized, and would remove this log message.

@metacosm in case could you explain how this help pls?

thank you!

cc @xstefank

@metacosm
Copy link
Collaborator

The use case is that not all ConfigurationService implementations should or want to create the configuration on demand. In some cases, having that method allows implementation to even throw an exception for example if the requested configuration is missing when it shouldn't be (because, for example, all configurations were supposed to have already been resolved at the call point).

@csviri
Copy link
Collaborator Author

csviri commented Aug 11, 2025

Yes, throwing exception in some cases would be fine, but we should not log warning. (In addition to that context of josdk it is expected that this method returns null in some cases.)

@metacosm
Copy link
Collaborator

Yes, throwing exception in some cases would be fine, but we should not log warning. (In addition to that context of josdk it is expected that this method returns null in some cases.)

And with the implementation in the other PR, no logging occurs.

@csviri
Copy link
Collaborator Author

csviri commented Aug 11, 2025

commented there, I don't see what is the problem we are solving with that PR. Pls choose where to continue the discussion :)

@csviri
Copy link
Collaborator Author

csviri commented Aug 11, 2025

replaced by: #2892

@csviri csviri closed this Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants